-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update ember-window-mock, drop unused glue code #399
Conversation
🦋 Changeset detectedLatest commit: 04d1800 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
79d7051
to
5bbca34
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yay!!
// NOTE: | ||
// - Location implementation is incomplete: | ||
// https://github.com/kaliber5/ember-window-mock/blob/2b8fbf581fc65e7f5455cd291497a3fdc2efdaf5/addon-test-support/-private/mock/location.js#L23 | ||
// - does not allow setting "origin" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see the changelog comment
// @ts-expect-error | ||
import locationFactory from 'ember-window-mock/test-support/-private/mock/location'; | ||
|
||
const AUGMENTS: Array<string | symbol> = ['origin']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see the changelog comment
get(target, key, receiver) { | ||
if (key === 'location') return location; | ||
if (key === 'parent') return self; | ||
if (key === 'top') return self; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This behaviour is now ensured upstream, see https://github.com/simonihmig/ember-window-mock/pull/575/files#diff-cbde1dc33d38be8541d8e3c47e25031991ff2948aca51f1ad25d20a8ecd875e5R38-R40
}, | ||
set(target, key, value, receiver) { | ||
if (key === 'location') { | ||
throw new Error(`location cannot be set on window`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not correct actually. See https://developer.mozilla.org/en-US/docs/Web/API/Window/location
Though Window.location is a read-only Location object, you can also assign a string to it. This means that you can work with location as if it were a string in most cases: location = 'http://www.example.com' is a synonym of location.href = 'http://www.example.com'.
ember-window-mock
itself always allowed this behaviour, see https://github.com/simonihmig/ember-window-mock/blob/master/test-app/tests/unit/run-window-tests.ts#L123-L129
module('Stubbing location.origin', function (hooks) { | ||
setupBrowserFakes(hooks, { | ||
window: { | ||
location: { origin: 'http://init.ial', href: 'http://init.ial' }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dropping this test can be seen as a breaking change, see the changelog comment
@NullVoxPopuli any concerns?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope, the migration path seems straight-forward!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
https://github.com/simonihmig/ember-window-mock/releases/tag/v1.0.0